Add AbstractMCMC.getstats and AbstractMCMC.requires_unconstrained_space
#182
Conversation
|
AbstractMCMC.jl documentation for PR #182 is available at: |
There was a problem hiding this comment.
It's not obvious to me what unconstrained should default to. I guess most samplers need it, so true makes sense from that point of view. I could also see an argument that false is the simpler choice, in that you get the values exactly as the model sees them. Although from a LDP point of view that doesn't really matter, it just takes floats and gives log densities. So happy with this, but paused to wonder about the default.
|
I would also be quite happy to leave out the default and force the user to specify it (this is already quite breaking for external samplers so another breaking change won't matter too much). |
|
I think, let's go with this for now, since the main drawback of linking (i.e. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #182 +/- ##
===========================
===========================
☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| Retrieve sampler statistics from the sampler's `state` as a `NamedTuple`. | ||
| """ | ||
| function getstats end |
There was a problem hiding this comment.
I'd hope we have more specific guidelines on how samplers should implement or use this API.
Following on from: - TuringLang/AbstractMCMC.jl#182 Adding the interface function `AbstractMCMC.getstats` - TuringLang/AdvancedHMC.jl#471 and TuringLang/AdvancedMH.jl#119 Implementing them for AdvancedHMC and AdvancedMH this PR changes Turing's external sampler interface to exclusively use AbstractMCMC functions. **With this PR, anyone who defines an external sampler will only need to depend on AbstractMCMC (which they presumably already do, because it is a sampler) and LogDensityProblems (which is already a dep of AbstractMCMC).** No need for a Turing extension. To be precise, it makes the following changes: - Previously where one had to define `Turing.Inference.getparams`, now one has to define `AbstractMCMC.getparams`. - Previously there was no way to include sampler stats in the resulting chain, now one can define `AbstractMCMC.getstats`. - The default for `Turing.Inference.isgibbscomponent` is changed to `true`, so that external sampler packages don't need to override it (unless absolutely necessary). As an example implementation, this PR contains a test mock (note how it doesn't require a Turing dep): https://github.com/TuringLang/Turing.jl/blob/06752c41a97cd0dc37bb8700ab7a2d06f50f4f76/test/mcmc/external_sampler.jl#L20-L74
In general there are two things that samplers return per iteration: parameter values, and sampler statistics.
getparamswas added previously to cover the first one, but there's no interface function for the second.Currently, for Turing's external sampler interface, external samplers are expected to implement
Turing.Inference.getparamsandTuring.Inference.getstatsThis is a mistake, because external samplers shouldn't need to depend on Turing, they should only depend on AbstractMCMC and LogDensityProblems (because they only need to take an
AbstractMCMC.LogDensityModeland callLogDensityProblems.logdensityon its wrapped model). I'm now trying to unravel this, hence the need for this function.In the same vein, this PR also moves
requires_unconstrained_spacefrom Turing to here. I'm less certain about this. Please see Slack for discussion. But I think that whether a sampler requires unconstrained space or not is a property of the sampler (and thus belongs in the package that defines AbstractSampler), not a property of how it interacts with Turing.